Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add definitionPath attribute to 'gdbtarget' JSON schema. #19

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

FlorentVSTM
Copy link

This PR adds the 'definitionPath' attribute by extending the 'gdbtarget' JSON schema from cdt-gdb-vscode, helping users and avoiding the yellow squiggly line.

Note this PR was first proposed in the cdt-gdb-vscode repository, but after discussion with Colin, it was decided that it is better to make the change here: eclipse-cdt-cloud/cdt-gdb-vscode#116

@thegecko
Copy link
Contributor

Thanks for the PR @FlorentVSTM

I would prefer to see this fixed in a more generic way for two reasons:

  • this fix is specific to one debugger. I would prefer debugger types don't bleed into this project nor have to add many more entries as we see similar issues in other debuggers
  • different debuggers may have this path available as a different config item, therefore its overridable in the settings. If a user changes this, your squiggles reappear

There may be a better way of fixing this using a DiagnosticCollection or similar

cc @colin-grant-work

@colin-grant-work
Copy link

It does look like the way this extension handles this field creates a challenge for JSON schema documentation, because it seems to say that it will assign special significance to whatever field of a config is set in this configuration. Since that isn't something that the debugger extension itself is guaranteed to know - as in the case of GDB, where it can be used without this extension and doesn't use the peripheral fields for itself - it doesn't make sense to document it as a feature of a particular debugger. On the other hand, since it's dynamic, it doesn't even make sense to add it to the schema here, because we don't know what value that field could take, or whether some debugger actually might already know and care about that field have documented it itself, which I assume is why the actual value of the field name is configurable, to allow it to be adjusted in case a debug adapter already defines it.

@FlorentVSTM that leaves you in a difficult position, because it doesn't seem like the functionality you're looking for quite belongs in either upstream project. If you have a mechanism of distributing a fork to your users, you could apply your fix to either extension downstream. Alternatively, we could raise a feature request on VSCode to allow extensions to contribute general schema additions, so that this extension could say 'any debug configuration may include field X' without specifying the specific debug types to which it applies. Even then, the dynamic nature of the key as its defined here would likely make a hard sell for the feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants